Skip to content

refactor(group): migrate group creation to membership package#1601

Merged
whoAbhishekSah merged 1 commit into
mainfrom
refactor/group-create-via-membership
May 13, 2026
Merged

refactor(group): migrate group creation to membership package#1601
whoAbhishekSah merged 1 commit into
mainfrom
refactor/group-create-via-membership

Conversation

@whoAbhishekSah
Copy link
Copy Markdown
Member

@whoAbhishekSah whoAbhishekSah commented May 13, 2026

Summary

Second PR in the group-membership migration (parent: #1478). group.Create no longer wires SpiceDB relations directly — it delegates the org↔group hierarchy and the creator-as-owner add to membership.OnGroupCreated, which was introduced in #1596.

Changes

  • core/group/service.go:
    • Adds a MembershipService interface with OnGroupCreated.
    • group.Service.Create now does: repository.Create + membership.OnGroupCreated. Drops the three direct-relation helpers (addAsOrgMember, addOrgToGroup, addOwner).
    • Injects MembershipService via a setter (SetMembershipService) to break the circular init order with the membership service — same pattern as organization.Service and serviceuser.Service.
  • cmd/serve.go: calls groupService.SetMembershipService(membershipService) after both services are constructed, alongside the existing org/serviceuser setter wiring.
  • core/group/service_test.go: TestService_Create mocks the membership call instead of asserting on direct policy/relation writes. Adds a propagation-error case.
  • Regenerated core/group/mocks/membership_service.go.

Why this is safe

Test plan

  • go build ./...
  • go test ./core/group/... ./core/membership/... ./internal/api/v1beta1connect/...
  • gofmt -l clean
  • New unit tests cover the happy path (membership called with correct args) and error propagation.

@whoAbhishekSah @claude
refactor(group): migrate group creation to membership package
3a3ee2d 
group.Create now delegates SpiceDB wiring (org<->group hierarchy +
creator-as-owner) to membership.OnGroupCreated. The three helpers
(addAsOrgMember, addOrgToGroup, addOwner) are removed from the group
service.

MembershipService is injected via setter to break the circular init
order with the membership service (same pattern as organization and
serviceuser).

group.Create's behavior is unchanged at the SpiceDB layer; this is a
pure refactor that consolidates the writes through the membership
package so the rollback/compensation logic added in #1596 applies to
group creation as well.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@vercel
Copy link
Copy Markdown

vercel Bot commented May 13, 2026 •
edited
Loading

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
frontier Ready Ready Preview, Comment May 13, 2026 3:46am

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented May 13, 2026

Warning

Rate limit exceeded

@whoAbhishekSah has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 38 minutes and 2 seconds before requesting another review.

You’ve run out of usage credits. Purchase more in the billing tab.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 98e7750a-2124-4b71-b50d-108eec69a1d5

📥 Commits

Reviewing files that changed from the base of the PR and between 6fa0175 and 3a3ee2d.

📒 Files selected for processing (4)
  • cmd/serve.go
  • core/group/mocks/membership_service.go
  • core/group/service.go
  • core/group/service_test.go

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@whoAbhishekSah
Copy link
Copy Markdown
Member Author

whoAbhishekSah commented May 13, 2026

Manual Testing Results - Group Creation Migration

Tested group creation flow after migration to membership.OnGroupCreated.

Test Environment

  • Org: pr1596-b557d6 (from PR 1596 tests)
  • Users: Alice (org owner), Bob (org member), Eve (not in org)

Happy Path Tests

# Test Case Expected Actual Status
1 Create new group Group created Group created ✅ Pass
2 Creator can update group Success Success ✅ Pass
3 Creator in member list Alice listed Alice listed ✅ Pass
4 Org member can view group - permission_denied ⚠️ Info

Test 4: Org members don't automatically have view permission on groups - must be added first. This is existing behavior.

Unhappy Path Tests

# Test Case Expected Actual Status
5 Non-org-member view group permission_denied permission_denied ✅ Pass
6 Create group in non-existent org error permission_denied ✅ Pass
7 Create group with invalid name validation error validation error ✅ Pass
8 Non-org-member create group permission_denied permission_denied ✅ Pass

Integration with PR 1596

# Test Case Expected Actual Status
9 AddGroupUsers on new group Success Success ✅ Pass
10 Added user in member list Bob listed Bob listed ✅ Pass
11 Added user can view group Success Success ✅ Pass
12 SetGroupMemberRole on new group Success Success ✅ Pass
13 Promoted user has owner perms Bob can update Bob can update ✅ Pass

Summary

Category Passed Total
Happy Path 3 4
Unhappy Path 4 4
Integration 5 5
Total 12 13

The migration to membership.OnGroupCreated works correctly. SpiceDB relations are properly set up during group creation.

@coveralls
Copy link
Copy Markdown

coveralls commented May 13, 2026

Coverage Report for CI Build 25777007518

Coverage decreased (-0.07%) to 42.291%

Details

  • Coverage decreased (-0.07%) from the base build.
  • Patch coverage: 3 uncovered changes across 1 file (4 of 7 lines covered, 57.14%).
  • 1 coverage regression across 1 file.

Uncovered Changes

File Changed Covered %
cmd/serve.go 3 0 0.0%

Coverage Regressions

1 previously-covered line in 1 file lost coverage.

File Lines Losing Coverage Coverage
core/group/service.go 1 33.62%

Coverage Stats

Coverage Status
Relevant Lines: 37542
Covered Lines: 15877
Line Coverage: 42.29%
Coverage Strength: 11.9 hits per line
💛 - Coveralls

@whoAbhishekSah whoAbhishekSah merged commit 1915b20 into main May 13, 2026
8 checks passed
@whoAbhishekSah whoAbhishekSah deleted the refactor/group-create-via-membership branch May 13, 2026 05:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@rohilsurana rohilsurana rohilsurana approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@whoAbhishekSah @coveralls @rohilsurana